Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix to wrap around onup scrolling fix in Preferences Issue #180 #16

Open
wants to merge 184 commits into
base: rasplex-dev
Choose a base branch
from
Open

Fix to wrap around onup scrolling fix in Preferences Issue #180 #16

wants to merge 184 commits into from

Conversation

guydev
Copy link

@guydev guydev commented Apr 15, 2014

Fix to wrap around onup scrolling fix in Preferences Issue xbmc#180
Also a cosmetic fix to align the left side and right side. Left arrow no points at left box instead of into empty space.

Tobias Hieta and others added 30 commits March 18, 2014 10:07
It turns out that doing operator overloading on classes that are
wrapped in shared_ptr’s are not a good idea. They will never be called
when you do if (sharedPtr == sharedPtr2) it will only test that the
reference is correct in both.

I have now removed the operator== on both PlexConnection and PlexServer
and implemented Equals() instead to use that. This has caused massive
problems with connection testing for a long time.
Don’t set CONNECTION_STATE_FAILED when we explicitly abort a connection
attempt, let it be unknown instead.
This might change stuff like view state and such.

Fixes xbmc#1067
This would before also run when we listed episodes etc.
LongChair and others added 10 commits April 11, 2014 12:53
When using always transcode subtitles, they would appear twice, this patch has been retrieved from upstream DVD player patches
This fixes the keyboard trap that existed.
As it is a Keyboard Numberic I removed the rest of the keyboard as it was uneccessary and causing problems.
Also a cosmetic fix to align height of left side and right side (arrow on left now points at left side instead of into empty space.
@LongChair
Copy link
Member

I can merge that one if you just fixthe few comments :) Thanks

@@ -319,7 +319,7 @@
<posy>20</posy>
<width>360</width>
<height>0</height>
<label>$INFO[ListItem.Label]</label>
<label>$INFO[ListItem.Title]</label>
<info>ListItem.Label</info>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmmz this looks suspicious .. i think it's a recent modification of upstream. we should probably not change that :)

@LongChair
Copy link
Member

Well Guy, sorry but i still have a couple comments as you will see. Just generally talking, Make sure that the skin file in the end contains no comment at all, as that will be an issue when PRIng.

In order to explain what the patch does, just comment in the commit message, that's the way upstream wants it to be.

In order to get you the picture also, I just wanna make sure that i can take your patch as it will land in our tree and be able to cherry pick it & PR it upstream. I had some troubles earlier with some PRs and i just wanna make sure this patch gets the optimal chances to be accepted as is :)

Another good thing would be to 'squash' your commit with git so that it doesn't end up in 8 commits.
Ideally it should end up with one well commented commit. Here is a link talking about squashing commits in case you dont know how to proceed : http://davidwalsh.name/squash-commits-git

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants